Skip to content

feat(project): implement add functionality. - #2004

Merged
Hweinstock merged 23 commits into
aws:refactorfrom
Hweinstock:feat/add-general
Aug 17, 2026
Merged

feat(project): implement add functionality. #2004
Hweinstock merged 23 commits into
aws:refactorfrom
Hweinstock:feat/add-general

Conversation

@Hweinstock

@Hweinstock Hweinstock commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Problem

Add functionality is not implemented.

Solution

  • nest project spec attributes within spec so project.managedBy becomes project.spec.managedBy to make updating the spec more ergonomic. We keep name at the top level.
  • implement add for harness using the generator pattern (will be more relevant for runtime).
  • swap to a discriminated union vs a flag config for more ergonomic typing. This allows a single parameter, and allows TS to fully infer the shape.

Testing

  • added unit tests that verify all the flags work as expected.
  • tested e2e by creating a project and adding a harness.

@github-actions github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 14, 2026
@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.75325% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.01%. Comparing base (01eb60b) to head (0fcefd8).
⚠️ Report is 1 commits behind head on refactor.

Files with missing lines Patch % Lines
src/core/project/manager.tsx 95.34% 4 Missing ⚠️
src/core/project/templates.ts 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           refactor    #2004    +/-   ##
==========================================
  Coverage     97.01%   97.01%            
==========================================
  Files           368      368            
  Lines         21630    21758   +128     
==========================================
+ Hits          20984    21109   +125     
- Misses          646      649     +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 14, 2026
@Hweinstock Hweinstock changed the title feat(project): implement core add functionality. feat(project): implement add functionality. Aug 14, 2026
Comment thread src/core/types.tsx
// endpoint, ...).
export interface AwsClients {
control(config: ClientConfig): BedrockAgentCoreControlClient
control(config: ClientConfig): BedrockAgentCoreControlClient;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps a rebase issue, but this is failing ci on main https://github.com/aws/agentcore-cli/actions/runs/31832012548/job/94869692163.

const RESULT: EvaluateResult = {
sessionsRequested: 1,
sessionsEvaluated: 1,
results: [{ evaluatorId: "Builtin.Helpfulness", value: 0.9 } as EvaluateResult["results"][number]],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

resourceConfig: z.input<typeof HarnessSpecSchema>;
}
| {
resourceType: "runtime";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added runtime just to start the pattern.

@Hweinstock
Hweinstock marked this pull request as ready for review August 14, 2026 20:48

@tejaskash tejaskash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few cases where the new add path reports success but writes a project that will not behave as requested. I left the details inline.

Comment thread src/core/project/manager.tsx Outdated
case "harness": {
yield { message: `Scaffolding harness in project` };
const harnessPath = await this.scaffoldHarness(project.rootPath, input.resourceConfig);
newResources.push({ name: input.resourceConfig.name, path: harnessPath });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep this path relative to the project root? scaffoldHarness returns an absolute path, so this writes the developer's local path into agentcore.json. It works on that machine, but after the project is moved or cloned, CDK still tries to read the old path. Writing app/${name} here would match the existing project format and keep the project portable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, let me swap that to the relative path.

Comment thread src/core/project/templates.ts Outdated
spec: z.input<typeof HarnessSpecSchema>,
): Promise<FsTreeNode> {
return FsTreeNode.createDirectory(".", [
FsTreeNode.createFile("harness.json", async () => json(HarnessSpecSchema.parse(spec))),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Dockerfile needs to be handled as part of this scaffold too. Right now --dockerfile only puts the filename in harness.json; this tree creates harness.json and system-prompt.md, but never copies the Dockerfile into app/<harness>. The command succeeds, then CDK uses this directory as the build context and cannot find the file. Can we validate and copy the source Dockerfile here, and store its basename like the existing CLI does?

@Hweinstock Hweinstock Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't aware thats how it worked, but that makes sense since it keeps the project portable. let me add that.

harnessConfig,
)) {
for await (const event of config.projectManager.addResource(project, {
resourceType: "harness",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part of the requested tool configuration gets lost before this call. For an agentcore_gateway tool, toTool copies gatewayArn but drops outboundAuth. For example, asking for { none: {} } produces a harness config with no outbound auth, which changes the behavior back to the default AWS IAM mode. Could we preserve the awsIam, none, and oauth variants?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think @notgitika spotted the same in #1998 (comment). Was going to address as follow-up, but let me bring in here.

)) {
for await (const event of config.projectManager.addResource(project, {
resourceType: "harness",
resourceConfig: harnessConfig,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private Git skill auth has a field mismatch here. The SDK input gives us a credentialArn, but toSkill writes that value into credentialName. During synth, CDK treats credentialName as a project credential key, so it cannot resolve the ARN-shaped value and deployment fails. Could we either accept a project credential name for this project command, or carry the ARN separately instead of renaming it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good catch, I don't think what I have is quite right. It looks like using name here is going against the pattern for harness, since gateway, browser, and memory references all leverage ARN here. AFAICT credential is the only name reference in the input shape. I think we should accept an ARN here to be consistent so I'll add it to the schema, and follow up with the CDK change to support.

"harness",
harnessConfig,
)) {
for await (const event of config.projectManager.addResource(project, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently no way to add a Dockerfile-backed harness in VPC mode. The environment conversion carries subnets and security groups, but the harness schema also requires networkConfig.vpcId for Dockerfile builds, and this handler has no --vpc-id input. The command always fails validation for that combination. Could we add an explicit VPC ID option and thread it into the resource config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow great edge case find, was not aware vpc id was required with VPC + docker. Let me add the explicit flag for this and make it clear.

newResources.push({ name: input.resourceConfig.name, path: harnessPath });
break;
}
// TODO: add limited special casing for runtime and default for other resources that proxy directly to spec changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we either implement the runtime branch here or leave runtime out of AddResourceInput for now? At the moment a runtime call falls through this switch, emits the Updating project spec message, writes the unchanged runtime list, and returns successfully. That silent success will be hard for callers to diagnose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine right? assuming we are implementing add runtime right after this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to be the next PR, but understand the current behavior is confusing. I'll have it throw early.

return FsTreeNode.createDirectory(".", [
FsTreeNode.createFile("harness.json", async () => json(HarnessSpecSchema.parse(spec))),
FsTreeNode.createFile(
"system-prompt.md",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we making the changes on the cdk side as well? resolveSystemPrompt() on there uses the inline systemPrompt first and onlyl reads system-prompt.md when that field is absent. so there are these 2 input sources. did we want that behavior to still exist?

what we are losing here is that there is no way a user will be warned if they make changes in their local md file here if they don't update the value from harness.json.

@Hweinstock Hweinstock Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, good callout, was not aware of that behavior. I feel like having two sources of system prompts might be unnecessary and confusing if we scaffold a system-prompt.md for them so I think stripping the system prompt field from the json to maintain backwards compatibility, and treat the file as source of truth might be the simplest path forward.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree!

Comment thread src/core/project/manager.tsx Outdated
}

yield { message: `Updating project spec file at '${agentCoreSpecPath}'` };
const newProjectSpec = await this.json.write(agentCoreSpecPath, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the order of operations here is:
create harness.json -> create system-prompt.md -> update agentcore.json

if the write fails, the harness files remain and retrying would fail because it "already exists". can we add some cleanup to address this edge case?

@notgitika notgitika left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tejaskash tejaskash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Hweinstock
Hweinstock merged commit 2dba459 into aws:refactor Aug 17, 2026
11 of 19 checks passed
@Hweinstock
Hweinstock deleted the feat/add-general branch August 17, 2026 17:36
aidandaly24 added a commit that referenced this pull request Aug 17, 2026
* feat(project): wire dev handler

* fix(dev): read runtimes from Project.spec after #2004

* fix(dev): address project dev review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants